Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Table Component/Rankings Page #658

Merged
merged 13 commits into from
Aug 9, 2023
Merged

Table Component/Rankings Page #658

merged 13 commits into from
Aug 9, 2023

Conversation

lowtorola
Copy link
Contributor

@lowtorola lowtorola commented Jul 6, 2023 •
edited
Loading

  • Adds fully generic BattlecodeTable.tsx element, with support for pagination via an optional BattlecodeTableBottomElement.tsx bottom element!
  • Creates Rankings page for viewing a list of all our lovely teams!

@lowtorola lowtorola marked this pull request as draft July 6, 2023 02:21
@lowtorola lowtorola changed the base branch from main to frontend2 July 6, 2023 02:21
@lowtorola lowtorola changed the title Table Component Table Component/Rankings Page Jul 6, 2023
@lowtorola
Copy link
Contributor Author

lowtorola commented Jul 6, 2023 •
edited
Loading

TODOS:

  • Make table actually 80% of window width
  • Only light mode on table 😢
  • Add loading safety to onPage for BattlecodeTableBottomElement
  • Add "No Data" component to put in table when data is loading
  • Make cursor change when hovering rows if onRowClick is present
  • Make row color change on hover
  • Add search

@lowtorola
Copy link
Contributor Author

Consider alternative to useEffect for query... maybe function called in useEffect first page render? then called on page/search?

@lowtorola
Copy link
Contributor Author

lowtorola commented Aug 3, 2023 •
edited
Loading

BOOM 💥 (I disagree with eslint my useEffect is good )

acrantel
acrantel reviewed Aug 4, 2023
View reviewed changes
Copy link
Member

@acrantel acrantel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great job! left comments about async/await preferences and moving the spinner into components/elements

should be good to merge after we address those, so just rerequest me for review after you make changes

frontend2/src/components/BattlecodeTable.tsx Outdated Show resolved Hide resolved
frontend2/src/views/Rankings.tsx Outdated Show resolved Hide resolved
frontend2/src/views/Rankings.tsx Outdated Show resolved Hide resolved
@acrantel
Copy link
Member

acrantel commented Aug 4, 2023

image
also, the BattlecodeTableBottomElement shows every page from 1-[num_pages]. let's make it so that it cuts off the overflow at some point

one way we could display page nav at the bottom of the table is this (we would probably display more page numbers around the page in our actual implementation):
if our page # is 1

  • 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, ... 200
    if our page # is 12
  • 1 ... 9, 10, 11, 12, 13, 14, 15 ... 200
    if our page # is 200
  • 1 ... 196, 197, 198, 199, 200

@lowtorola lowtorola marked this pull request as ready for review August 4, 2023 03:20
@lowtorola lowtorola requested a review from acrantel August 6, 2023 06:40
acrantel
acrantel approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@acrantel acrantel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested the new table bottom element and it looks great! left some comments regarding the return type / documentation of the getPageNumbers function, but once that is addressed then this LGTM

frontend2/src/components/BattlecodeTableBottomElement.tsx
@@ -20,6 +20,38 @@ const BattlecodeTableBottomElement: React.FC<TableBottomProps> = ({
const backDisabled = currentPage <= 1;
const forwardDisabled = currentPage >= pageCount;

function getPageNumbers(): Array<string | number> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a comment here describing what this function does and its the return value? I see it returns strings and numbers but i'm not sure what that means

frontend2/src/components/BattlecodeTableBottomElement.tsx
.concat(["...", pageCount]);
}
} else if (pageCount < 1) {
return ["1"];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like we're returning a string containing 1 here instead of the number 1. is this intentional?

@lowtorola lowtorola merged commit 7dd1620 into frontend2 Aug 9, 2023
2 of 3 checks passed
@lowtorola lowtorola deleted the table-component branch August 9, 2023 00:47
acrantel added a commit that referenced this pull request Aug 15, 2023
@lowtorola @acrantel
Table Component/Rankings Page (#658)
5485173 
Co-authored-by: Serena Li <serena.li.usa@gmail.com>
acrantel added a commit that referenced this pull request Aug 15, 2023
@lowtorola @acrantel
Table Component/Rankings Page (#658)
e36dae5 
Co-authored-by: Serena Li <serena.li.usa@gmail.com>
acrantel added a commit that referenced this pull request Oct 3, 2023
@lowtorola @acrantel
Table Component/Rankings Page (#658)
cc86743 
Co-authored-by: Serena Li <serena.li.usa@gmail.com>
acrantel added a commit that referenced this pull request Feb 8, 2024
@lowtorola @acrantel
Table Component/Rankings Page (#658)
b35e8ce 
Co-authored-by: Serena Li <serena.li.usa@gmail.com>
acrantel added a commit that referenced this pull request Feb 8, 2024
@lowtorola @acrantel
Table Component/Rankings Page (#658)
67cb3ed 
Co-authored-by: Serena Li <serena.li.usa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acrantel acrantel acrantel approved these changes

@apollo1291 apollo1291 Awaiting requested review from apollo1291

@n8kim1 n8kim1 Awaiting requested review from n8kim1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lowtorola @acrantel